-
Notifications
You must be signed in to change notification settings - Fork 432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refreshable file data source #86
refreshable file data source #86
Conversation
6ab8eda
to
6e51178
Compare
c6b1fc1
to
5a69066
Compare
} | ||
|
||
func (s *RefreshableFileDataSource) Close() error { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Close
op should be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense.
The semantic of Close is to close the datasource and stop listen on dynamic datasource.
watcher *fsnotify.Watcher | ||
} | ||
|
||
func FileDataSourceStarter(sourceFilePath string, handlers ...datasource.PropertyHandler) (*RefreshableFileDataSource, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name starter
might be confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rename to NewFileDataSource
|
||
func (s *RefreshableFileDataSource) ReadSource() ([]byte, error) { | ||
f, err := os.Open(s.sourceFilePath) | ||
defer f.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be put after the error checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right
defer f.Close() | ||
|
||
if err != nil { | ||
return nil, errors.Errorf("The rules file is not existed, err: %+v.", errors.WithStack(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not just indicate non-exist, but also other circumstances like permission denied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, I will refine the statement.
return nil | ||
} | ||
|
||
func (s *RefreshableFileDataSource) doUpdate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about doReadAndUpdate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense.
for _, h := range s.Handlers() { | ||
err := h.Handle(src) | ||
if err != nil { | ||
logger.Errorf("RefreshableFileDataSource fail to publish rules, handle: %+v; err: %+v.", h, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) Here we could use the word "data" instead of rule
. (2) Printing the handler here might not be helpful. Maybe we need to print out the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) Right;
(2) Is type the datasource type?
s.doUpdate() | ||
w, err := fsnotify.NewWatcher() | ||
if err != nil { | ||
return errors.Errorf("Fail to new a w of fsnotify, err: %+v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
watcher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's caused by rename, I will rename here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's caused by mis rename. I will fix here.
|
||
func (s *RefreshableFileDataSource) Initialize() error { | ||
if !s.isInitialized.CompareAndSet(false, true) { | ||
return errors.New("RefreshableFileDataSource had been Initialized.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just returning "nil" is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
fee5480
to
407c196
Compare
|
||
func (s *RefreshableFileDataSource) Close() error { | ||
s.closeChan <- struct{}{} | ||
logger.Info("The RefreshableFileDataSource had been closed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also print the filePath here to make it clear to identify?
407c196
to
a853dac
Compare
Maybe we could also add the case for file-deleted event? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Nice work. Thanks! |
Describe what this PR does / why we need it
refreshable file data source implementation.
enhance the datasource framework:
a) Add return value error for PropertyConverter function to indicate whether occur error when converting.
b)Add return value error for datasource. ReadSource function to indicate whether occur error when reading source.
refreshable file datasource implementation
add plugin module.
a) This module is design to encapsulate converter(default json) and updater related logic.
Does this pull request fix one issue?
Resolves #84
Describe how you did it
Describe how to verify it
Unit test
Special notes for reviews